wolfcrypt polish: init, checks, corrections by gojimmypi · Pull Request #6249 · wolfSSL/wolfssl

gojimmypi · 2023-03-30T12:07:50Z

Description

A variety of minor cleanup and initialization issues to appease compiler complaints, addressed while developing the updates in my ED25519_SHA2_fix branch. Those changes have grown so numerous, I'm breaking down the updates into manageable PR's. 1/n

aes.c - return error if lengths for known-not-defined keylen values encountered.
asn.c - return zero (an error when dynamic memory allocation fails) for wc_EncodeSignature param lengths less than or equal to zero
cmac.c - check cmac->bufferSz > AES_BLOCK_SIZE in wc_CmacFinal
ed25519.c - cleanup. variable init.
md5.c - check md5 buffer length in wc_Md5Final.
random.c - include <esp_random.h> when ESP-IDF v5 detected. Add comments. Correct spelling of ENTROPY_NUM_WORDS_BITS
ripemd.c - check ripemd buffLen is not larger than RIPEMD_BLOCK_SIZE in wc_RipeMdFinal.

Fixes zd# n/a

Testing

I've been using these changes in the Espressif development for some time. Here's a script to run the wolfssl testwolfcrypt from WSL Ubuntu on Windows:

git clone https://github.com/gojimmypi/wolfssl.git wolfssl-pr1
cd wolfssl-pr1
git checkout wolfcrypt-updates

./autogen.sh

# enable-all test 
./configure CC=clang  --enable-all CFLAGS=-DHAVE_STACK_SIZE && make && ./wolfcrypt/test/testwolfcrypt

Unit testing

git clone https://github.com/gojimmypi/wolfssl.git wolfssl-6249
cd wolfssl-6249
./autogen.sh
./configure --enable-all --enable-asn=original
make
./scripts/unit.test

Checklist

added tests
updated/added doxygen
updated appropriate READMEs
Updated manual and documentation

kaleb-himes · 2023-03-30T13:41:14Z

retest this please

gojimmypi · 2023-03-31T09:34:46Z

inadvertently closed; sync with upstream & squashed commits.

re-opening for review.

gojimmypi · 2023-04-10T05:16:56Z

Jenkins retest this please

dgarske · 2023-04-11T16:20:01Z

wolfcrypt/src/aes.c


+#if !defined(WOLFSSL_AES_128)
+        if (keylen == 16) {
+          return BAD_FUNC_ARG;


Indents should be 4 spaces.

ah yes. I know that well. My bad for letting this one slip by. I've pushed an update to fix

wolfcrypt/src/cmac.c

wolfcrypt/src/ed25519.c

dgarske · 2023-04-11T20:10:11Z

wolfcrypt/src/cmac.c

 {
    int ret;
    const byte* subKey;
+    word32 remainder = 0;


You may get a set but not used with 0 init. Recommend removing the =0. This can also move into the else section. Variable declarations must be at top of function or brace section.

I've removed the init = 0 and left the declaration at the top of the function.

dgarske · 2023-04-13T18:57:24Z

The Jenkins error is:

Testing DEFAULT: --enable-sp-math-all --enable-cryptonly --enable-keygen --enable-smallstack 
wolfcrypt/src/sp_int.c: In function ‘_sp_exptmod_nct’:
wolfcrypt/src/sp_int.c:277:39: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare]
  277 |                 for (n##ii = 1; n##ii < (c); n##ii++) {                        \
      |                                       ^
wolfcrypt/src/sp_int.c:13245:5: note: in expansion of macro ‘ALLOC_SP_INT_ARRAY’
13245 |     ALLOC_SP_INT_ARRAY(t, m->used * 2 + 1, (size_t)preCnt + 2, err, NULL);
      |     ^~~~~~~~~~~~~~~~~~

However your code doesn't touch it. Consider a rebase to latest master?

wolfcrypt/src/md5.c

…crypt-updates

gojimmypi · 2023-04-14T09:37:01Z

The Jenkins error is:

Testing DEFAULT: --enable-sp-math-all --enable-cryptonly --enable-keygen --enable-smallstack 
wolfcrypt/src/sp_int.c: In function ‘_sp_exptmod_nct’:
wolfcrypt/src/sp_int.c:277:39: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare]
  277 |                 for (n##ii = 1; n##ii < (c); n##ii++) {                        \
      |                                       ^
wolfcrypt/src/sp_int.c:13245:5: note: in expansion of macro ‘ALLOC_SP_INT_ARRAY’
13245 |     ALLOC_SP_INT_ARRAY(t, m->used * 2 + 1, (size_t)preCnt + 2, err, NULL);
      |     ^~~~~~~~~~~~~~~~~~

However your code doesn't touch it. Consider a rebase to latest master?

Good idea. I've refreshed from upstream, but encountered a new error. I suspect trying again might be successful, but perhaps @bandi13 would be interested in reviewing this build timeout error first:

./configure --enable-jobserver=4 CC="clang -fsanitize=address,undefined -g" --enable-all
make[2]: warning: -j4 forced in submake: resetting jobserver mode.
make[3]: warning: -j4 forced in submake: resetting jobserver mode.
make[3]: warning: -j4 forced in submake: resetting jobserver mode.
make[4]: warning: -j4 forced in submake: resetting jobserver mode.
Build timed out (after 120 minutes). Marking the build as aborted.
$ ssh-agent -k
unset SSH_AUTH_SOCK;
unset SSH_AGENT_PID;
echo Agent pid 3837301 killed;
[ssh-agent] Stopped.
Build was aborted

gojimmypi · 2023-04-18T10:46:23Z

Jenkins retest this please

gojimmypi · 2023-04-18T15:40:46Z

Hi @dgarske the refresh from upstream worked. Looks like GitHub has been having some recently problems, perhaps this explains prior unexpected and odd Jenkins failures.

In any case, please review and let me know if you'd like any other changes.

* wolfcrypt polish: init, checks, corrections

gojimmypi requested review from SparkiDev, dgarske and douzzer March 30, 2023 12:07

gojimmypi force-pushed the wolfcrypt-updates branch from b5ff2b0 to 0418a74 Compare March 30, 2023 13:07

gojimmypi mentioned this pull request Mar 30, 2023

ESP32 Espressif Improvements - Roadmap Summary #6234

Closed

72 tasks

gojimmypi marked this pull request as draft March 30, 2023 16:10

gojimmypi closed this Mar 31, 2023

gojimmypi force-pushed the wolfcrypt-updates branch from 8ea8e9a to 0210dcd Compare March 31, 2023 09:00

wolfcrypt polish: init, checks, corrections

078bd39

gojimmypi reopened this Mar 31, 2023

gojimmypi self-assigned this Mar 31, 2023

gojimmypi marked this pull request as ready for review April 1, 2023 04:39

gojimmypi assigned dgarske Apr 11, 2023

dgarske requested changes Apr 11, 2023

View reviewed changes

dgarske removed their assignment Apr 11, 2023

wolfcrypt polish: init, checks, corrections; code review updates

464760c

gojimmypi requested a review from dgarske April 11, 2023 17:54

gojimmypi assigned dgarske Apr 11, 2023

dgarske requested changes Apr 11, 2023

View reviewed changes

dgarske removed their assignment Apr 11, 2023

wolfSSL#6249 code review updates.

6183c9d

gojimmypi requested a review from dgarske April 13, 2023 07:33

gojimmypi assigned dgarske Apr 13, 2023

dgarske requested changes Apr 13, 2023

View reviewed changes

wolfcrypt/src/md5.c Show resolved Hide resolved

dgarske removed their assignment Apr 13, 2023

Merge branch 'master' of https://github.com/wolfSSL/wolfssl into wolf…

73b2cb1

…crypt-updates

gojimmypi requested a review from dgarske April 18, 2023 15:40

gojimmypi assigned dgarske Apr 18, 2023

dgarske approved these changes Apr 18, 2023

View reviewed changes

dgarske merged commit 98b718f into wolfSSL:master Apr 18, 2023

joeftiger pushed a commit to joeftiger/wolfssl that referenced this pull request Sep 2, 2023

wolfcrypt polish: init, checks, corrections (wolfSSL#6249)

034da78

* wolfcrypt polish: init, checks, corrections

Conversation

gojimmypi commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Unit testing

Checklist

Uh oh!

kaleb-himes commented Mar 30, 2023

Uh oh!

gojimmypi commented Mar 31, 2023

Uh oh!

gojimmypi commented Apr 10, 2023

Uh oh!

dgarske Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

gojimmypi Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dgarske Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

gojimmypi Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

dgarske commented Apr 13, 2023

Uh oh!

Uh oh!

gojimmypi commented Apr 14, 2023

Uh oh!

gojimmypi commented Apr 18, 2023

Uh oh!

gojimmypi commented Apr 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gojimmypi commented Mar 30, 2023 •

edited

Loading